Skip to content

Emit list() for mvcombine so it runs on the analytics engine - #5663

Closed
noCharger wants to merge 1 commit into
opensearch-project:mainfrom
noCharger:fix/ppl-mvcombine-list
Closed

Emit list() for mvcombine so it runs on the analytics engine#5663
noCharger wants to merge 1 commit into
opensearch-project:mainfrom
noCharger:fix/ppl-mvcombine-list

Conversation

@noCharger

@noCharger noCharger commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Description

mvcombine lowered to Calcite ARRAY_AGG(<field>) FILTER (<field> IS NOT NULL), which the analytics-engine (DataFusion) route cannot execute: ARRAY_AGG is not in the engine's aggregate function set, so the plan is rejected during marking. Mapping it into the engine is not viable either — Calcite ARRAY_AGG infers a nullable-element ARRAY while the engine's array_agg infers a NOT NULL element, and the two cannot be reconciled during the aggregate rewrite.

The fix stays on the SQL side: performArrayAggAggregation now emits the existing PPL list() aggregate (PPLBuiltinOperators.LIST), which the analytics engine already supports end to end (it maps to the engine's array_agg / list_merge operators). ListAggFunction filters nulls internally, so the explicit IS NOT NULL filter is dropped. No analytics-engine (core) change is required.

Behavior change (please review): the combined field is now ARRAY<VARCHAR>list() renders elements as strings and retains at most 100 values per group, per its documented contract. For string target fields (the documented mvcombine use) output is unchanged; numeric/other targets are now stringified and capped. On the Calcite/Lucene path this differs from the previous unbounded, type-preserving ARRAY_AGG, so flagging in case reviewers prefer to gate it.

Bug reproduce (analytics-engine / DataFusion route)

Composite/parquet index, analytics.planner.prefer_metadata_driver=false (forces DataFusion):

source=test_data | fields category, status | mvcombine status

Before (ARRAY_AGG) — rejected on the analytics engine

Logical plan:

LogicalProject(category=[$0], status=[$1])
  LogicalAggregate(group=[{0}], status=[ARRAY_AGG($1) FILTER $2])
    LogicalProject(category=[$0], status=[$7], $f2=[IS NOT NULL($7)])
      LogicalTableScan(table=[[opensearch, test_data]])

Physical plan: none — rejected during analytics-engine marking, before execution.

Output:

{
  "error": {
    "reason": "There was internal problem at backend",
    "details": "No enum constant org.opensearch.analytics.spi.AggregateFunction.ARRAY_AGG",
    "type": "IllegalArgumentException"
  },
  "status": 500
}

After (LIST) — runs on DataFusion

Logical plan:

LogicalProject(category=[$0], status=[$1])
  LogicalAggregate(group=[{0}], status=[LIST($1)])
    LogicalProject(category=[$0], status=[$7])
      LogicalTableScan(table=[[opensearch, test_data]])

Physical plan (profile=true; every node viableBackends=[[datafusion]], stage chosen_backend=datafusion):

OpenSearchSort(fetch=[10000], viableBackends=[[datafusion]])
  OpenSearchProject(category=[$0], status=[$1], viableBackends=[[datafusion]])
    OpenSearchAggregate(group=[{0}], status=[LIST($1)], mode=[SINGLE], viableBackends=[[datafusion]])
      OpenSearchProject(category=[$0], status=[$7], viableBackends=[[datafusion]])
        OpenSearchTableScan(table=[[test_data]], viableBackends=[[datafusion]])

Output:

+----------+--------------------------+
| category | status                   |
|----------+--------------------------|
| A        | ["active", "active"]     |
| B        | ["inactive", "active"]   |
| C        | ["pending"]              |
+----------+--------------------------+

Related Issues

Related to #4766 ([FEATURE] Add mvcombine command in PPL) — extends it to the analytics-engine (DataFusion) route.

Check List

  • New functionality includes testing.
    • CalcitePPLMvCombineTest logical-plan + Spark SQL expectations updated for the LIST aggregate. CalciteMvCombineCommandIT is unchanged (it already asserts string-array output on a string field).
  • New functionality has been documented.
    • docs/user/ppl/cmd/mvcombine.md gains a Limitations note (string elements, 100-value cap, unordered).
  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit d779cc2)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Breaking change

The documented limitation that values are capped at 100 per group represents a breaking change for existing users who may rely on combining more than 100 values. Queries that previously aggregated unbounded arrays will now silently truncate results beyond 100 elements, potentially causing data loss in production workloads without user awareness.

## Limitations

Values are combined using the `list()` aggregation: the combined field is a multivalue array of **strings** (non-string values are rendered as strings), at most **100** values are retained per group, and the order of values within the array is not guaranteed.
Type coercion risk

Switching from ARRAY_AGG to LIST changes the output type from type-preserving to ARRAY. For numeric fields, this means values like 123 become "123". Downstream consumers expecting numeric arrays (e.g., for mathematical operations or type-strict integrations) will break. The PR description flags this as a behavior change but does not indicate whether existing queries or integrations have been validated against this type shift.

// list() rather than Calcite ARRAY_AGG: list() carries the ARRAY<VARCHAR> contract the
// analytics-engine (DataFusion) route supports, and drops nulls itself (no filter needed).
final RelBuilder.AggCall aggCall =
    relBuilder.aggregateCall(PPLBuiltinOperators.LIST, targetRef).as(targetName);

@noCharger
noCharger force-pushed the fix/ppl-mvcombine-list branch from 5007125 to 9c91c9b Compare July 30, 2026 15:22
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9c91c9b

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to d779cc2
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add data loss warning

The 100-value limit per group is a critical constraint that could cause silent data
loss. Consider adding a warning about potential data truncation and recommend users
verify their data doesn't exceed this limit before using mvcombine.

docs/user/ppl/cmd/mvcombine.md [113]

 Values are combined using the `list()` aggregation: the combined field is a multivalue array of **strings** (non-string values are rendered as strings), at most **100** values are retained per group, and the order of values within the array is not guaranteed.
 
+**Warning**: If a group contains more than 100 values, excess values will be silently dropped. Verify your data doesn't exceed this limit to avoid data loss.
+
Suggestion importance[1-10]: 8

__

Why: This suggestion addresses a critical documentation gap regarding the 100-value limit that could lead to silent data loss. Adding an explicit warning about potential data truncation is important for user awareness and preventing unexpected behavior. The improved documentation helps users make informed decisions about using mvcombine.

Medium
Possible issue
Verify LIST operator registration

Verify that PPLBuiltinOperators.LIST is properly defined and registered in the
system. The removal of the null filter assumes LIST handles nulls internally, but if
this operator is not correctly implemented or registered, queries will fail at
runtime.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4502-4503]

+// Ensure PPLBuiltinOperators.LIST is registered and handles null filtering
 final RelBuilder.AggCall aggCall =
     relBuilder.aggregateCall(PPLBuiltinOperators.LIST, targetRef).as(targetName);
Suggestion importance[1-10]: 7

__

Why: This suggestion asks to verify that PPLBuiltinOperators.LIST is properly defined and registered. While this is a valid concern for ensuring runtime correctness, it's a verification request rather than identifying a concrete issue in the PR code itself. The score reflects its importance for system stability but acknowledges it's not addressing a definite bug.

Medium

Previous suggestions

Suggestions up to commit 9c91c9b
CategorySuggestion                                                                                                                                    Impact
Possible issue
Verify null handling in LIST aggregation

The removal of the null filter may cause issues if PPLBuiltinOperators.LIST doesn't
handle nulls as expected. Verify that ListAggFunction actually drops nulls
internally as stated in the comment, or add explicit null handling to prevent
potential runtime errors or incorrect aggregation results.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4504-4505]

+// Verify ListAggFunction null handling behavior
 final RelBuilder.AggCall aggCall =
     relBuilder.aggregateCall(PPLBuiltinOperators.LIST, targetRef).as(targetName);
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that removing the null filter relies on ListAggFunction handling nulls internally. While the comment states this behavior, verification is prudent. However, the 'improved_code' only adds a comment without actual verification logic, limiting its practical impact.

Medium

mvcombine lowered to Calcite ARRAY_AGG with an explicit IS NOT NULL
filter. The analytics-engine (DataFusion) route cannot execute that
shape: Calcite ARRAY_AGG infers a nullable-element ARRAY while the
engine's array_agg operator infers a NOT NULL element, and the two
cannot be reconciled during the aggregate rewrite.

Switch performArrayAggAggregation to emit the existing PPL list()
aggregate (PPLBuiltinOperators.LIST), which the analytics engine already
supports. ListAggFunction drops null values internally, so the explicit
IS NOT NULL filter is no longer needed.

Behavior note: the combined field is now ARRAY<VARCHAR> (list()
stringifies elements) and is capped at 100 values per group, matching
list()'s documented contract.

CalcitePPLMvCombineTest logical-plan and SparkSQL expectations updated
for the LIST aggregate.

Document the list()-based output (strings, 100-value cap, unordered) in
mvcombine.md.

Also update CalciteExplainIT explain_mvcombine.yaml fixtures (pushdown
on/off) for the LIST plan (drop the IS NOT NULL projection; the identity
EnumerableCalc is eliminated on the pushdown path).

Signed-off-by: Louis Chu <lingzhichu.clz@gmail.com>
@noCharger
noCharger force-pushed the fix/ppl-mvcombine-list branch from 9c91c9b to d779cc2 Compare July 30, 2026 15:36
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit d779cc2

@noCharger noCharger self-assigned this Jul 30, 2026
@noCharger noCharger closed this Jul 30, 2026
@noCharger

Copy link
Copy Markdown
Collaborator Author

Closed in favor of a type-preserving fix on the analytics-engine (DataFusion) side.

This PR emitted the PPL list() aggregate, which runs on the DataFusion route but downgrades the combined field to ARRAY<VARCHAR> (elements stringified) with a 100-value cap on all engines — a semantic regression for non-string fields.

Instead, mvcombine keeps emitting the type-preserving Calcite ARRAY_AGG (mainline; already correct on Calcite/Lucene), and the DataFusion route is taught to execute it via a dedicated type-preserving array-agg operator (element type = source type, no VARCHAR cast). Java-only in the analytics engine, no Rust change (the array_agg/list_merge UDAFs are already type-generic).

Verified live on the DataFusion route: mvcombine over an integer field returns ARRAY<INT> ([30, 30], not ["30","30"]) with chosen_backend=datafusion; list()/values() remain ARRAY<VARCHAR>. That change will be raised as a separate opensearch-project/OpenSearch (core) PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant